test(ltx-2.5): the register boundary, made numerically reachable (#435) - #597
test(ltx-2.5): the register boundary, made numerically reachable (#435)#597localai-bot wants to merge 1 commit into
Conversation
FOLLOWING_AGENTS_PROTOCOL A fresh review of the L9c connector PR passed the change and found a hole in its gate. `prompt_embeds_valid_rows` is the single knob deciding which token positions become the connector's TRAINED `learnable_registers` and which carry caller-supplied conditioning -- on the accepted render, 104 of 128 rows are registers. The reviewer changed `s = im.prompt_valid_rows` to `s = im.prompt_valid_rows + 1` at `ltx2_video.cpp:744` and `test_ltx2_video` stayed fully green: 23/23 cases, 253/253 assertions, exit 0. Reproduced here before anything was written. WHY THE COVERAGE COULD NOT SEE IT. The only case on that knob asserted that a render at valid=4 differs from one at valid=2. ANY monotone corruption of the boundary keeps that true, so the whole family passes it. The defect it cannot see is ONE padded row conditioned on caller junk instead of a trained register: finite, correctly shaped, plausible, and wrong. WHAT MAKES THE BOUNDARY REACHABLE IS THE SUBSTITUTION, NOT THE MASK. `binary_mask * hidden_states + (1 - binary_mask) * registers` (embeddings_connector.py:148-150) REPLACES a masked position's features, so what the caller supplied there cannot reach the render at all. The mask carries nothing: with registers on, :152 returns `torch.zeros_like(mask)`, and `_to_binary_mask`'s `encoded_mask < 0.000001` (embeddings_processor.py:46-48) is satisfied by BOTH values an additive mask holds, so what the DiT receives is all ones either way. Positions therefore separate by whether perturbing them moves a byte, and the boundary is pinned from BOTH sides: rows [valid, N) are REGISTERS -> perturbing them must change NOTHING rows [0, valid) are CAPTION -> perturbing the last one must change something The polarity is `(attention_mask - 1) * finfo.max` (transformer_args.py:203-206) and the contiguity is `_compute_right_pad_order`'s stable descending argsort (embeddings_processor.py:33-38), which is what makes the padded set the suffix `[valid, N)` that the extra names. RED FIRST, on the product code, restored byte-for-byte after each run (`git diff` empty, HEAD unchanged, verified per mutation): + 1 "every row at or past the count is a register" FAILS 110/111 - 1 "the row just BEFORE the count is caption" FAILS 110/111 0 both caption arms FAIL 109/111 v_rows "every row at or past the count is a register" FAILS 110/111 and `+ 1` over the WHOLE suite: 24 cases, 23 passed, 1 failed; 364 assertions, 363 passed, 1 failed. Only the new case moves, which is the finding reproduced and closed in one run. TWO SIBLING FINDINGS FOLDED IN. F3 is a correction of a claim, and then a gate under it. The L9c commit message said `Ltx2ParseConnectorConfig` "mirrors both configurators key for key". It does not: `connector_num_learnable_registers` is read here and by NEITHER configurator (embeddings_connector.py:194-219 and :222-256 both take the class default of 128). The divergence is deliberate and stays -- a checkpoint declaring something else must not be silently run at 128 -- but a read nothing can falsify is a claim rather than a gate, so it now has one: the fixture's stored table stays `[2, dim]` while the config declares 4, and `Ltx2LoadConnectorWeights`' shape check fires. REDs when the read is made inert. The correction itself lands in `porting-inventory.md` and in the test, NOT as a comment beside the parse. `check-doc-checkpoint` classifies any path under `src/vllm/model_executor/models/` as `feature_surface` and would demand a `docs/FEATURES.md` entry for a comment that changes no feature -- and inventing one to satisfy a checker is a false record. The parse's existing comment already says "NOT read by either configurator", so nothing there was untrue; what was missing was the gate, and the gate is what this adds. F4 is an unreachable branch that named the wrong reason. The refusal on a masked cross-attention mask said it fires "when the connector runs with num_learnable_registers = 0". It cannot: with registers disabled the caller's additive mask passes straight through and `< 0.000001` is still true at every position, so the branch is unreachable on every input either reference produces. The comment and the message now say that, name what WOULD reach it, and say why it is not gated. Also in the record: `porting-inventory.md` cited "mutation M3/M4" by labels that existed only in a PR body, which git cannot resolve -- replaced with the test case names, which are in the tree. NOT REPRODUCED, and recorded rather than dropped: the shipped-checkpoint case does not skip silently. `MESSAGE: SKIPPED: set LTX2_CHECKPOINT_ROOT ...` prints in the binary's own output, in the baseline run above. It IS invisible under `ctest`, which captures a passing test's output, and its env is deliberately not the tree-wide `CHECKPOINT_ROOT` -- `.env.example` says that one "declares an INTENT, not a behaviour" and nothing reads it. Both are now stated at the case so a green ctest row cannot be read as "the shipped checkpoints were read". Gate: clean-configured CPU-only Release build, BUILD_EXIT=0, no `No space left`/`BFD assertion`/kill in the log, `ctest -N` 399 tests. Issue: #435 Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
…lly was one Collapses the L9 stack. The register-gate fix (#597, which closed the reviewer's prompt_valid_rows finding two-sided) is already folded in below this. Two conflicts, and they wanted opposite treatments, which is the whole reason the "union it" reflex is wrong on record files. .agents/benchmark-record.md IS genuinely append-only: both sides appended distinct sections at the same anchor, nothing keyed collided. Unioned in chronological order, older (the 2026-08-11 BENCHMARKS.md overflow move) before newer (L9c's drain measurement), and verified both survive. docs/FEATURES.md is the same single keyed row for the THIRD time this campaign, and again neither side was a superset. L9C knew the render had become a scene at 320x192/25f with the connector wired; L9B knew the coverage had grown to both VAE encoders and the conditioning items, and that torchao-NVFP4 loads while the first-party file does not. Composed the facts true of the merged tree. The row then failed check-public-doc-tables at 221 chars in one cell against the 220 cap -- worth recording that the budget bites at exactly the point a merge tempts you to concatenate two descriptions instead of writing one. Issue: #435 FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5 [Claude Code]
|
Landed on Closing rather than merging because the base branch it targeted ( The finding it closed is worth restating, since it is the fourth instance of one class in this campaign: mutating |
Closes review finding F1 (MEDIUM) on PR #574, plus the sibling F3 (LOW) and F4 (INFO). Issue: #435.
Base is
row/LTX25-L9C-CONNECTOR-DRAINat708e05b56.The gap
prompt_embeds_valid_rowsis the single knob deciding which token positions become the connector's trainedlearnable_registers(104 of 128 rows on the accepted render). A fresh reviewer changeds = im.prompt_valid_rowstos = im.prompt_valid_rows + 1atsrc/vllm/multimodal/ltx2_video.cpp:744andtest_ltx2_videostayed fully green — 23/23 cases, 253/253 assertions, exit 0. Reproduced here before anything was written.The existing case asserts only that a render at
valid=4differs from one atvalid=2. Any monotone corruption of the boundary keeps that true. The defect it cannot see is one padded row conditioned on caller junk instead of a trained register: finite, correctly shaped, plausible, wrong.The closure
The substitution — not the mask — is what carries the boundary.
binary_mask * hidden_states + (1 - binary_mask) * registers(embeddings_connector.py:148-150) replaces a masked position's features, so what the caller supplied there cannot reach the render. The mask carries nothing either way::152returnstorch.zeros_like(mask), and_to_binary_mask's< 0.000001(embeddings_processor.py:46-48) is satisfied by both values an additive mask holds.So positions separate by whether perturbing them moves a byte, and the boundary is pinned from both sides:
[valid, N)are registers → perturbing them must change nothing[0, valid)are caption → perturbing the last one must change somethingPolarity from
(attention_mask - 1) * finfo.max(transformer_args.py:203-206); contiguity from_compute_right_pad_order's stable descending argsort (embeddings_processor.py:33-38), which is what makes the padded set the suffix the extra names.RED evidence
Each mutation applied to the product code, built and run in one chain, then restored —
git diffempty and HEAD unchanged after every one.prompt_valid_rows + 1Status: FAILURE!, exit 1prompt_valid_rows - 1Status: FAILURE!, exit 10Status: FAILURE!, exit 1v_rowsStatus: FAILURE!, exit 1And
+ 1over the whole suite:24 | 23 passed | 1 failed,364 | 363 passed | 1 failed. Only the new case moves — the reviewer's finding reproduced and closed in one run.Siblings
F3.
Ltx2ParseConnectorConfigreadsconnector_num_learnable_registers; neither configurator does (embeddings_connector.py:194-219,:222-256both take the class default of 128), so "key for key" is not literally true of that key. The divergence is deliberate and stays, but a read nothing can falsify is a claim rather than a gate — so it now has one: the fixture's stored table stays[2, dim]while the config declares 4, and the shape check fires. REDs when the read is made inert (4/5assertions).The correction lands in
porting-inventory.mdand the test, not as a comment beside the parse:check-doc-checkpointclassifiessrc/vllm/model_executor/models/asfeature_surfaceand would demand adocs/FEATURES.mdentry for a comment that changes no feature. The parse's existing comment already says "NOT read by either configurator".F4. The masked-mask refusal claimed it fires at
num_learnable_registers = 0. It cannot — with registers disabled the additive mask passes straight through and< 0.000001is still true at every position. The comment and message now say the branch is unreachable on every input either reference produces, name what would reach it, and say why it is not gated.porting-inventory.md's "mutation M3/M4" labels existed only in a PR body; replaced with the test case names, which are in the tree.Not reproduced: the shipped-checkpoint case does not skip silently —
MESSAGE: SKIPPED: set LTX2_CHECKPOINT_ROOT ...prints in the baseline run. It is invisible underctest's output capture, and its env is deliberately not the tree-wideCHECKPOINT_ROOT(.env.example: that one "declares an INTENT, not a behaviour" and nothing reads it). Both now stated at the case.Gate
Clean-configured CPU-only Release build (
-DCMAKE_BUILD_TYPE=Release -DVLLM_CPP_CUDA=OFF), rebuilt from a wipedCMakeCache.txt/CMakeFiles:BUILD_EXIT=0,enospc_hits=0,werror_hits=0,df89% throughoutctest -N→Total Tests: 399ctest -j4→100% tests passed, 0 tests failed out of 399,CTEST_EXIT=0test_ltx2_video→24 | 24 passed | 0 failed,365 | 365 passed | 0 failed,Status: SUCCESS!(baseline was23 | 23,253 | 253)check-doc-checkpoint.py --base 708e05b56 --head HEAD→OK🤖 Generated with Claude Code